Conversation
andrewleap-optimizely
left a comment
There was a problem hiding this comment.
Looking good! Appreciate the thorough tests. Just a few suggestions/changes requested
jaeopt
left a comment
There was a problem hiding this comment.
Looks good! A few changes and clarifications suggested.
andrewleap-optimizely
left a comment
There was a problem hiding this comment.
Looking good! One more small fix.
jaeopt
left a comment
There was a problem hiding this comment.
All changes look good! A couple of more clarifications.
optimizely/odp/odp_manager.py
Outdated
| segment_manager.odp_config = self.odp_config | ||
| self.segment_manager = segment_manager | ||
| else: | ||
| self.segment_manager = OdpSegmentManager(self.odp_config, |
There was a problem hiding this comment.
Can we consider removing odp_config from the constructor params? It should be overriden for custom segment-manager anyway? Let's talk about it when integrating at the top level.
jaeopt
left a comment
There was a problem hiding this comment.
I see an error in cache timeout.
| return | ||
| if self.odp_config.odp_state() == OdpConfigState.NOT_INTEGRATED: | ||
| self.logger.debug('ODP Identify event is not dispatched (ODP not integrated).') | ||
| self.logger.debug('ODP identify event is not dispatched (ODP not integrated).') |
There was a problem hiding this comment.
Line 107-114, send_event raises multiple exceptions. They should be processed separately at the top level. We can discuss again when integrated.
Summary
Test plan
Issues